r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

Note that this'll fail CI until we get a nightly tonight, but I was testing with a toolchain that has support for # env-dep so I think everything should otherwise be working.

Ok I've pushed up a commit which fixes some dep-info parsing bits in the test suite, but this actually uncovered an unfortunate behavior that I'm not sure what to do about:

#[cargo_test]
fn env_build_script_no_rebuild() {
    // Only nightly has support in dep-info files for this
    if !cargo_test_support::is_nightly() {
        return;
    }
    let p = project()
        .file(
            "Cargo.toml",
            r#"
                [package]
                name = "foo"
                version = "0.1.0"
            "#,
        )
        .file(
            "build.rs",
            r#"
                fn main() {
                    println!("cargo:rustc-env=FOO=bar");
                }
            "#,
        )
        .file(
            "src/main.rs",
            r#"
                fn main() {
                    println!("{:?}", env!("FOO"));
                }
            "#,
        )
        .build();

    p.cargo("build").run();
    p.cargo("build").with_stderr("[FINISHED] [..]").run();
}

That test fails because it rebuilds. The problem is that Cargo sees the previous compilation read the env var FOO, but it's reading the value of FOO from Cargo's own environment, which misses how this env var is defined by a build script. A similar issue is happening in the target dir renaming test because the value of OUT_DIR is changing, and that's not defined in Cargo's process.

Cargo can't really predict the value of env vars coming from build scripts, so I guess it just has to ignore them? We'd have to keep a list of "these env vars are defined by build scripts" from the previous compilation and use that to filter the env vars to check in Cargo itself.

I'll try to think about this and see if there's something better. Handling OUT_DIR I think will also be a bit tricky.

Well, the behavior I would expect is that we don't necessarily rebuild the crate after re-running the build script -- I think today that's never the case, though, so would be "new feature" in some sense. I think I recall an issue about this but can't find it today.

I think that sort of thing is pretty hard given Cargo's architecture today -- IIRC, we need to upfront decide everything we're planning to build. I guess in theory the crate could be a no-op build somewhat hackily (the cargo 'step' runs but compiler isn't invoked).

Ok I've pushed up a commit I think addresses the issue. I figured out an easy-ish way to do it because translate_dep_info happens so late in the process.

FWIW we unconditionally rebuild the crate if a build script is rerun, we definitely don't have any red/green style tracking like the compiler does where if the build script produces the same output we don't rebuild any crates. (same for if you recompile a crate and it produces byte-for-byte the same output we still build everything else)

The main problem I was imagining is that we can't even predict what env vars a build script will set. At the point we translate rustc's dep info to Cargo's dep info, however, we have a ProcessBuilder which contains all specific env vars to just that compilation which we can exclude from rustc's set of recorded env vars used during the compile. That should limit us precisely to the set of env vars used that weren't defined by Cargo, which is what we're interested in tracking. (the comment inline has more info to this)

Ok updated!

Not sure if you saw the note about changing the version in compute_metadata. I feel it would be a little safer to change that.

Er oops sorry I saw that and then completely forgot to add it, I'll look into escaping, I don't know what kind of escaping rustc does...

Ok read up on the escaping rustc does and am now matching that here too

Thanks, looks good!

@bors r+

📌 Commit 0750a6f has been approved by ehuss

⌛ Testing commit 0750a6f with merge 77b4058...

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 77b4058 to master...